-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(agentcore): add agentcore runtime L2 construct #35623
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/perms.ts
Outdated
Show resolved
Hide resolved
/** | ||
* Agent runtime artifact configuration for Bedrock Agent Core Runtime | ||
*/ | ||
export interface AgentRuntimeArtifactConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we expecting this configuration to have more elements in the future? Do you know if there are plans for that? Seems like having this interface at the moment is not adding too much value, you could potentially return the URI directly in the bind method rather than this object (in CDK, we tend to favor flat composition over nested composition if we can)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mirrors the cloudformation structure of agentruntimeartifact . IMHO, we should keep it as cloud formation structure because if the underlying service add any new field in this object then the flat structure of containeruri
may cause breaking changes for the L2 construct.
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/runtime.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/runtime.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/types.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/runtime.ts
Outdated
Show resolved
Hide resolved
* @param clientId Cognito App Client ID | ||
* @param region Optional AWS region (defaults to stack region) | ||
*/ | ||
public configureCognitoAuth( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the other methods below are exactly what I was expecting to see instead of https://github.com/aws/aws-cdk/pull/35623/files#r2398909386.
Simple and specific for each type of auth configuration. I would prefer to keep only this idea and drop the possibility for the user to set up manually the auth config with all the possible wrong combinations they could make if we let it fully configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/runtime-endpoint-base.ts
Outdated
Show resolved
Hide resolved
public readonly endpointName: string; | ||
public readonly agentRuntimeArn: string; | ||
public readonly status?: string; | ||
public readonly targetVersion?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference between this and the agentRuntimeVersion
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually have two of those liveVersion and targetVersion. I have added liveVersion with this new commit.
this.liveVersion = this.endpointResource.attrLiveVersion;
this.targetVersion = this.endpointResource.attrTargetVersion;
https://docs.aws.amazon.com/bedrock-agentcore/latest/devguide/agent-runtime-versioning.html
As per my understanding
Initial Creation :
- You set
agentRuntimeVersion: "1"
- Both
liveVersion
andtargetVersion
will be "1" - Status: READY
During Update :
- Update runtime version to "2"
liveVersion
remains "1"targetVersion
becomes "2" (transitioning to this)- Status: UPDATING
After Update Completes :
- Both
liveVersion
andtargetVersion
become "2" - Status: READY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont mind having this if CFN also provides it, but I also dont get how this is useful. CDK does not allow interaction "during updates'. In CDK, this will generate a reference to a value that is actually available only after this resource is deployed, and by that time, both liveVersion
and targetVersion
will be have the same value. Again, I dont mind having it, since CFN is providing it, but lets make the documentation as clear as possible about the differences of both fields
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/runtime-endpoint.ts
Show resolved
Hide resolved
* | ||
* @returns RuntimeAuthorizerConfiguration for IAM authentication | ||
*/ | ||
public static configureIAM(): RuntimeAuthorizerConfiguration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to: usingIAM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
* @param allowedAudience Optional array of allowed audiences | ||
* @returns RuntimeAuthorizerConfiguration for JWT authentication | ||
*/ | ||
public static configureJWT( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to usingJWT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
* @param region Optional AWS region where the User Pool is located (defaults to stack region) | ||
* @returns RuntimeAuthorizerConfiguration for Cognito authentication | ||
*/ | ||
public static configureCognito( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to usingCognito
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
* @param scopes Optional array of OAuth scopes | ||
* @returns RuntimeAuthorizerConfiguration for OAuth authentication | ||
*/ | ||
public static configureOAuth( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to usingOAuth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
provider: string, | ||
discoveryUrl: string, | ||
clientId: string, | ||
scopes?: string[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
provider
and scopes
parameters are ignored in the constructor if the OAuthAuthorizerConfiguration
class. Why do we need them here then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently OAuthAuthorizerConfiguration
and JwtAuthorizerConfiguration
are behaving same way under the hood . In my view usingOAuth
provides a better user experience (having a dedicated usingOAuth
method makes it clear that developers are configuring OAuth providers like GitHub or Google, even though the underlying AWS service treats it the same as JWT authentication). I kept provider
and scopes
for potential future enhancements where we might add OAuth-specific features or some metadata for observability.
I'm happy to remove these unused parameters to keep the code cleaner. Would you prefer I remove them, or keep them documented as reserved for future use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets remove them for now, since they are not used. If we add features that need then, we can introduce them later. In the future, if more specific configuration is required for each OAuth providers, we can introduce special methods, like usingGoogleOAuth
or something like that, and in there we request the additional data. From a user perspective, if the name of the method already gives you the provider, is very clear what is the intention behind it, and with the current approach we can manage to introduce this later when needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
* Optional description for the agent runtime endpoint | ||
* @default - No description | ||
*/ | ||
readonly description?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CFN has a pattern for this field, lets add it here too please (JSDoc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
* A list of key:value pairs of tags to apply to this RuntimeEndpoint resource | ||
* @default {} - no tags | ||
*/ | ||
readonly tags?: { [key: string]: string }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CFN has a pattern for this field, lets add it here too please (JSDoc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
value: this.description, | ||
fieldName: 'Description', | ||
minLength: 1, | ||
maxLength: 1200, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In CFN doc, the max length is 256
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh they changed it. Will update thanks.
const account = Stack.of(this).account; | ||
|
||
// ECR Image Access - scoped to account repositories | ||
role.addToPolicy(new iam.PolicyStatement({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This policy is needed if the runtime artifact is fromEcrRepository
right? Could we check if the agentRuntimeArtifact
is of type EcrImage
to add this policy? And maybe even restrict the resource to that specific one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was following these permission as per this . However , I realized that ECR permissions are redundant for L2 construct because these are already handled with AgentRuntimeArtifact
bind function with grantPull
. The ecr grantPull
take care of permissions in best possible way. We need these permission during bind so that it can pull the image from ecr repository before deploying it on runtime. I have removed these permission from createExecutionRole
and have tested the changes.
})); | ||
|
||
// ECR Token Access - must be * for authorization token | ||
role.addToPolicy(new iam.PolicyStatement({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the previous policy, if only needed when artifact is fromEcrRepository
, we should try and restrict the creation only in that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above , removed the permission as they were redundant and using ecr grantPull
to grant the permission.
…ws-cdk into agentcore-runtime
Issue # (if applicable)
Related to aws/aws-cdk-rfcs#785
Reason for this change
Adding bedrock agent core runtime and runtime endpoint
Description of changes
Describe any new or updated permissions being added
The runtime creates a role with permission to ecr repo, cloudwatch , xray .
Description of how you validated changes
Unit tests, integration tests, manual tests
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license